Allow TypeNotFound parser errors #957

bergmeister · 2018-03-31T17:06:52Z

PR Summary

Fixes #850
Some Types can not be known in some cases (e.g. due to using statements because the parser does not load anything by design), therefore those TypeNotFound parser errors are being ignored, since the AST can still be parsed.
Thanks to @rjmholt for helping with parser related questions.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
User facing documentation needed
Change is not breaking
Make sure you've added a new test if existing tests do not effectively test the code changed
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

kalgiz · 2018-04-05T00:28:19Z

Engine/ScriptAnalyzer.cs

@@ -1644,6 +1646,31 @@ private static Encoding GetFileEncoding(string path)
            }
        }

+        /// <summary>
+        /// Inspects Parse errors and removes TypeNotFound errors that can be ignored that can be due to types that are not known yet (e.g. due to 'using' statements)


Mistake in sentence.

Maybe something like:
/// Inspects Parse errors and removes TypeNotFound errors that can be ignored since some types are not known yet (e.g. due to 'using' statements)

kalgiz · 2018-04-05T00:37:24Z

Tests/Engine/InvokeScriptAnalyzer.tests.ps1

+            It "does not throw and detect one expected warning after the parse error has occured when using -ScriptDefintion parameter set" {
+                $warnings = Invoke-ScriptAnalyzer -ScriptDefinition $script
+                $warnings.Count | Should -Be 1 -Because 'PSSA should analyze the whole script after the parse error on [IStorageContext] and find the AvoidAlias warning due to gci'
+            }


Maybe we should check this warning is from rule 'AvoidAliases' directly?

kalgiz · 2018-04-05T00:37:35Z

Tests/Engine/InvokeScriptAnalyzer.tests.ps1

+            Set-Content $testFilePath -value $script
+            It "does not throw and detect one expected warning after the parse error has occured when using -Path parameter set" {
+                $warnings = Invoke-ScriptAnalyzer -Path $testFilePath
+                $warnings.Count | Should -Be 1 -Because 'PSSA should analyze the whole script after the parse error on [IStorageContext] and find the AvoidAlias warning due to gci'


Same as above.

…nalyzer into AllowTypeNotFoundParserErrors

… downtime

…nalyzer into AllowTypeNotFoundParserErrors

JamesWTruher

i think it's close, but we need a bit more discussion on this because I think it might be overreaching a bit.

JamesWTruher · 2018-04-09T18:12:12Z

Engine/ScriptAnalyzer.cs

+                else
+                {
+                    this.outputWriter.WriteVerbose(
+                        $"Ignoring 'TypeNotFound' parse error on type {parseError.Extent}, which is likely due to the type not being knowm due to e.g. 'using' statements. Parse error message was '{parseError.Message}'.");


should this be a resource rather than a static string?

JamesWTruher · 2018-04-09T18:32:59Z

Tests/Engine/InvokeScriptAnalyzer.tests.ps1

+                using namespace Microsoft.Azure.Commands.ResourceManager.Cmdlets.SdkModels
+                using namespace Microsoft.Azure.Commands.Common.Authentication.Abstractions
+                Import-Module "AzureRm"
+                class MyClass { [IStorageContext]$StorageContext } # This will result in a parser error due to [IStorageContext] type that comes from the using stetement but is not known at parse time


typo - stetement should be statement.

JamesWTruher · 2018-04-09T18:35:12Z

Tests/Engine/InvokeScriptAnalyzer.tests.ps1

+            It "does not throw and detect one expected warning after the parse error has occured when using -ScriptDefintion parameter set" {
+                $warnings = Invoke-ScriptAnalyzer -ScriptDefinition $script
+                $warnings.Count | Should -Be 1 -Because $assertionMessage
+                $warnings.RuleName | Should -Be 'PSAvoidUsingCmdletAliases' -Because $assertionMessage


I know that there a bunch of these sorts of tests which check the message, but I'm not a fan. If we want to have tests for the message catalog, we can do that explicitly. Is there another way to check this? Checking a rulename is not subject to localization.

This is just checking for the rule type/name, not the message itself, the -Because operator is for specifying additional details in the assertion message why the test was expected to fail if it fails, not an assertion.

JamesWTruher · 2018-04-09T18:58:52Z

Engine/ScriptAnalyzer.cs

@@ -1522,16 +1522,18 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini
                return null;
            }

-            if (errors != null && errors.Length > 0)
+            var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors);


Should there be an escape hatch here? Is there a scenario where a user would like to see all of the parse errors? I have a feeling that this rule is actually useful (as it catches type errors like [System.Stirng]).

I see, good point. Maybe rather output a warning instead of a verbose message? Or even return a non-terminating error?
@tylerl0706 Does VSCode\PowerShellEditorServices do its own parsing or is it the PSSA errors that we see in the editor when the type is unknown/invalid?

I changed it to output a warning for the moment.

The errors aren't terminating anyway, except that when you get too many of them PSSA gives up. When scanning some of the Azure packages for the gallery (https://dtlgalleryint.cloudapp.net/packages/AzSKStaging/3.1.2 is an example), we get about 4000 errors like this, so I'd really welcome some option to suppress them.

@edyoung Do you get other errors apart from the TypeNotFound error as well? This PR removes all of them and only outputs them as a warning instead. Yes, when there are more than 10 non-TypeNotFound, then PSSA will throw a terminating error. Maybe an environment variable to tweak this magic value? I cannot download your referenced module because it is not in the public PSGallery, sorry.

I think it's fine to terminate parsing after 10 errors, but TypeNotFound issues not treated as errors shouldn't count towards that limit. Right now those are the only messages I get for that package but in general there will be other errors.

Actually you should be able to download that package if you like:

Register-PSRepository -Name GalleryRolling -SourceLocation https://dtlgalleryint.cloudapp.net/api/v2/ Save-Module -Repository GalleryRolling -Name AzSKStaging -AllowPreRelease -Path whatever

I tried it with your branch, and the errors are converted to warnings as expected.

Yes, TypeNotFound does not count towards that limit, this PR changes it to exactly that, only a warning (like Write-Warning not a rule violation) is emitted to the console host. Thanks for the snippet.

the problem with warnings/errors/et al, is that they're missed in output. Would an Information diagnostic record be ok? That way, they would be part of the output, but can easily be suppressed.

…nalyzer into AllowTypeNotFoundParserErrors

…hat runs only on v5+

…g in this test script

…sage

…tom rule name and tweak error message

…rename variable

bergmeister added 5 commits March 31, 2018 17:29

Trim out TypeNotFoundParseErrors.

fd787e1

Add verbose logging and test

5d2f5d4

fix test syntax

8a2826e

add test for -path parameter set

a24e185

use pester test drive

bc31332

bergmeister self-assigned this Mar 31, 2018

bergmeister added the Area - Engine label Mar 31, 2018

bergmeister added this to the 1.17 milestone Mar 31, 2018

fix tests to not run in library usage session

e4dc844

bergmeister requested a review from JamesWTruher March 31, 2018 17:37

kalgiz reviewed Apr 5, 2018

View reviewed changes

bergmeister added 3 commits April 5, 2018 08:16

Address PR comments about additional assertion and xml comment

d54d531

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

d80ba89

…nalyzer into AllowTypeNotFoundParserErrors

Remove unused dotnet-core NuGet feed that caused build failure due to…

0575548

… downtime

kalgiz approved these changes Apr 5, 2018

View reviewed changes

bergmeister added 2 commits April 5, 2018 20:24

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

b639486

…nalyzer into AllowTypeNotFoundParserErrors

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

817a99f

…nalyzer into AllowTypeNotFoundParserErrors

JamesWTruher reviewed Apr 9, 2018

View reviewed changes

bergmeister added 10 commits April 9, 2018 20:10

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

ae63e6a

…nalyzer into AllowTypeNotFoundParserErrors

Fix typo and move string into resources

3564b8e

fix compilation error (forgot to check-in file)

321582e

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

e8a91ad

…nalyzer into AllowTypeNotFoundParserErrors

fix typo to also build for psv3

259a730

Since the using statement was introduced only inn v5, move to block t…

897e426

…hat runs only on v5+

fix tests by moving the tests (clearly a smell that something is wron…

3201b9a

…g in this test script

write warning on parse errors instead of verbose output and tweak mes…

9792f96

…sage

instead of returning a warning, return a diagnostic record with a cus…

9636d43

…tom rule name and tweak error message

poke build due to sporadic AppVeyor failure on setup (not a test) -> …

2b44732

…rename variable

JamesWTruher approved these changes May 11, 2018

View reviewed changes

JamesWTruher merged commit ac22b0e into PowerShell:development May 11, 2018

rjmholt mentioned this pull request May 29, 2018

Issues with 'Using Module' and classes PowerShell/vscode-powershell#1076

Closed

avalery mentioned this pull request Jul 19, 2018

TypeNotFound suppressions #1041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow TypeNotFound parser errors #957

Allow TypeNotFound parser errors #957

bergmeister commented Mar 31, 2018 •

edited

Loading

kalgiz Apr 5, 2018

kalgiz Apr 5, 2018

kalgiz Apr 5, 2018

JamesWTruher left a comment

JamesWTruher Apr 9, 2018

JamesWTruher Apr 9, 2018

JamesWTruher Apr 9, 2018

bergmeister Apr 9, 2018

JamesWTruher Apr 9, 2018

bergmeister Apr 9, 2018 •

edited

Loading

bergmeister Apr 10, 2018

edyoung Apr 10, 2018

bergmeister Apr 10, 2018 •

edited

Loading

edyoung Apr 11, 2018

edyoung Apr 11, 2018

bergmeister Apr 11, 2018 •

edited

Loading

JamesWTruher Apr 11, 2018

Allow TypeNotFound parser errors #957

Allow TypeNotFound parser errors #957

Conversation

bergmeister commented Mar 31, 2018 • edited Loading

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWTruher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister commented Mar 31, 2018 •

edited

Loading

bergmeister Apr 9, 2018 •

edited

Loading

bergmeister Apr 10, 2018 •

edited

Loading

bergmeister Apr 11, 2018 •

edited

Loading